Automated Test: topic-email-management #309
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a topic-level unsubscribe feature for Discourse users. The implementation spans frontend components (new route, controller, view, template), backend actions (controller and model methods), email notification enhancements, routing configuration, localization strings, and UI styling to enable users to stop receiving notifications for specific topics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/models/topic_user.rb (1)
120-125:⚠️ Potential issue | 🟠 MajorPre-existing bug:
track_visit!usestopicinstead ofuserin fallback.Line 122: when
useris not aUserinstance, the fallback assignstopictouser_idinstead ofuser. Since this PR touches the surrounding method (line 125), flagging for awareness.user_id = user.is_a?(User) ? user.id : topic # should be `user`, not `topic`Proposed fix
- user_id = user.is_a?(User) ? user.id : topic + user_id = user.is_a?(User) ? user.id : userapp/assets/javascripts/discourse/routes/app-route-map.js.es6 (1)
8-13:⚠️ Potential issue | 🔴 CriticalRoute ordering conflict prevents
topicUnsubscribefrom being reached.The nested
topic.fromParamsNearroute at/:nearPost(expands to/t/:slug/:id/:nearPost) will match/t/:slug/:id/unsubscribewithnearPost="unsubscribe"before the router considers the root-leveltopicUnsubscriberoute. SincefromParamsNear.setupController()expectsnearPostto be a numeric post ID, it will fail when attemptingpostStream.closestPostForPostNumber("unsubscribe").To fix:
- Define
topicUnsubscribeas a nested route inside thetopicresource (ensuring it matches beforefromParamsNear), or- Use a path prefix that won't collide, such as
/t/:slug/:id/-/unsubscribelib/email/message_builder.rb (1)
104-108:⚠️ Potential issue | 🟠 Major
List-Unsubscribeheader doesn't use the new topic-specificunsubscribe_url.The
List-Unsubscribeheader still points touser_preferences_url(line 107), even whenunsubscribe_urlis provided. Per RFC 2369, this header should ideally contain the most direct unsubscribe mechanism. When a topic-specificunsubscribe_urlis available, it would provide a better user experience.Proposed fix
def header_args result = {} if `@opts`[:add_unsubscribe_link] - result['List-Unsubscribe'] = "<#{template_args[:user_preferences_url]}>" if `@opts`[:add_unsubscribe_link] + unsubscribe = `@opts`[:unsubscribe_url].present? ? "#{Discourse.base_url}#{`@opts`[:unsubscribe_url]}" : template_args[:user_preferences_url] + result['List-Unsubscribe'] = "<#{unsubscribe}>" endspec/components/email/message_builder_spec.rb (1)
167-182:⚠️ Potential issue | 🟠 Major
unsubscribe_urlis passed to the builder but never actually used.The new
unsubscribe_url: "/t/1234/unsubscribe"parameter is added totemplate_argsvia the@opts.merge!at initialization, but it is never referenced in the email output:
- The
unsubscribe_linktemplate (inserver.en.yml) uses%{user_preferences_url}, not%{unsubscribe_url}- The
List-Unsubscribeheader usesuser_preferences_urlonly- Neither test verifies that
unsubscribe_urlis actually used anywhereThe test on line 179 passes because it checks for
user_preferences_url, which appears in the unsubscribe template regardless of the new parameter. Either remove the unusedunsubscribe_urlparameter from the test, or update the implementation to actually use it in place ofuser_preferences_url.
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs`:
- Around line 1-7: Rename the misspelled property stopNotificiationsText to
stopNotificationsText in the template and the corresponding controller
(topic-unsubscribe controller) so both places use the correct identifier; update
the template's {{{stopNotificiationsText}}} to {{{stopNotificationsText}}} and
the controller export/property that supplies it, keeping the triple-stache
unchanged because the i18n string intentionally contains HTML and is sanitized
server-side.
In `@app/controllers/topics_controller.rb`:
- Around line 98-116: The unsubscribe action currently mutates
TopicUser.notification_level in the controller method unsubscribe (using
TopicView and TopicUser.find_by) while being routed as a GET; change the route
to a non-safe verb (POST or PUT) and split the controller so GET unsubscribe
renders a confirmation page (no state changes) and the actual mutation happens
only in a new POST/PUT handler (e.g., unsubscribe_confirm or the same
unsubscribe action responding to POST/PUT). Move the logic that looks up tu
(TopicUser.find_by(...)) and updates tu.notification_level and tu.save! into the
POST/PUT branch, ensure CSRF protection is enabled for the form submission, and
keep perform_show_response only in the mutating handler.
- Around line 105-113: TopicUser.find_by can return nil, so guard the code that
reads/sets tu.notification_level: after calling TopicUser.find_by(user_id:
current_user.id, topic_id: params[:topic_id]) check whether tu is nil and handle
it (either create a new TopicUser for current_user and params[:topic_id] before
toggling notification_level, or return a suitable response like head :not_found
or redirect), then perform the existing toggle logic using
TopicUser.notification_levels and call tu.save! only when tu is present.
In `@app/views/email/notification.html.erb`:
- Line 10: In the H4 element in app/views/email/notification.html.erb (the line
rendering t "user_notifications.previous_discussion"), remove the erroneous
leading dot from the class attribute so the element uses the literal class name
previous-discussion (i.e., change class='.previous-discussion' to
class='previous-discussion') so CSS rules targeting the .previous-discussion
selector will match correctly.
- Around line 5-20: The template renders %{respond_instructions} twice when
context_posts.present? is true; fix by removing the inner duplicate (the <div
class='footer'>%{respond_instructions}</div> inside the if block) or
alternatively make the bottom footer conditional with unless
context_posts.present? so %{respond_instructions} only appears once; update the
view where context_posts, respond_instructions, and unsubscribe_link are used to
ensure only a single respond_instructions footer is rendered.
In `@config/routes.rb`:
- Around line 439-442: The slugless summary route "get \"t/:topic_id/summary\"
=> \"topics#show\"" is missing defaults so params[:summary] won't be set; update
that route to include defaults: { summary: true } (preserving the existing
constraints: {topic_id: /\d+/}) so topics#show receives the same summary flag as
the slug-based route.
🧹 Nitpick comments (3)
lib/email/message_builder.rb (1)
61-68: RuboCop: assignment in condition on line 62.The static analysis tool flags
if response_instructions =@template_args[:respond_instructions]as an assignment-in-condition. While this is a pre-existing pattern, wrapping in parentheses clarifies the intent and suppresses the warning.Proposed fix
- if response_instructions = `@template_args`[:respond_instructions] + if (response_instructions = `@template_args`[:respond_instructions])app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 (1)
1-9: ObjectController is deprecated in Ember and should be migrated on version upgrade.Ember 1.11.3 (currently in use) includes ObjectController with deprecation warnings present in the vendor code. While the codebase still functions with it, this is a systemic pattern used in 24+ instances across the application—migration should be coordinated as part of an Ember version upgrade strategy rather than addressed file-by-file. The
modelproxy behavior of ObjectController can be replaced with explicit getters on a standardEmber.Controllerwhen upgrading.app/controllers/topics_controller.rb (1)
27-28: Trailing comma missing after last array element.RuboCop flags this (
Style/TrailingCommaInArrayLiteral). Adding a trailing comma keeps diffs cleaner when appending more items.Proposed fix
- :bookmark, - :unsubscribe] + :bookmark, + :unsubscribe,]
| <div class="container"> | ||
| <p> | ||
| {{{stopNotificiationsText}}} | ||
| </p> | ||
| <p> | ||
| {{i18n "topic.unsubscribe.change_notification_state"}} {{topic-notifications-button topic=model}} | ||
| </p> |
There was a problem hiding this comment.
Typo in property name: stopNotificiationsText → stopNotificationsText.
The property name has an extra 'i' ("Notificiations" instead of "Notifications"). This is consistent with the controller so it works, but it should be fixed in both places to avoid confusion for future maintainers.
Also, the triple-stache {{{ }}} renders unescaped HTML. This is needed because the i18n string contains <strong> tags, and fancyTitle is sanitized server-side, so the XSS risk is mitigated. Worth noting for future reference.
Proposed fix
- {{{stopNotificiationsText}}}
+ {{{stopNotificationsText}}}Also fix in the controller (topic-unsubscribe.js.es6):
- stopNotificiationsText: function() {
+ stopNotificationsText: function() {🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs` around
lines 1 - 7, Rename the misspelled property stopNotificiationsText to
stopNotificationsText in the template and the corresponding controller
(topic-unsubscribe controller) so both places use the correct identifier; update
the template's {{{stopNotificiationsText}}} to {{{stopNotificationsText}}} and
the controller export/property that supplies it, keeping the triple-stache
unchanged because the i18n string intentionally contains HTML and is sanitized
server-side.
| def unsubscribe | ||
| @topic_view = TopicView.new(params[:topic_id], current_user) | ||
|
|
||
| if slugs_do_not_match || (!request.format.json? && params[:slug].blank?) | ||
| return redirect_to @topic_view.topic.unsubscribe_url, status: 301 | ||
| end | ||
|
|
||
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | ||
|
|
||
| if tu.notification_level > TopicUser.notification_levels[:regular] | ||
| tu.notification_level = TopicUser.notification_levels[:regular] | ||
| else | ||
| tu.notification_level = TopicUser.notification_levels[:muted] | ||
| end | ||
|
|
||
| tu.save! | ||
|
|
||
| perform_show_response | ||
| end |
There was a problem hiding this comment.
State mutation via GET request violates HTTP semantics.
The unsubscribe action modifies TopicUser.notification_level but is routed as a GET (see config/routes.rb lines 440–441). GET requests must be safe/idempotent per HTTP spec. This also exposes the action to CSRF via link prefetching, browser preloading, and crawler visits — any of which could silently toggle a user's notification state.
Consider changing the route to PUT or POST, and rendering a confirmation page on GET that submits via a form.
🧰 Tools
🪛 RuboCop (1.84.0)
[convention] 98-116: Assignment Branch Condition size for unsubscribe is too high. [<4, 31, 6> 31.83/23]
(Metrics/AbcSize)
🤖 Prompt for AI Agents
In `@app/controllers/topics_controller.rb` around lines 98 - 116, The unsubscribe
action currently mutates TopicUser.notification_level in the controller method
unsubscribe (using TopicView and TopicUser.find_by) while being routed as a GET;
change the route to a non-safe verb (POST or PUT) and split the controller so
GET unsubscribe renders a confirmation page (no state changes) and the actual
mutation happens only in a new POST/PUT handler (e.g., unsubscribe_confirm or
the same unsubscribe action responding to POST/PUT). Move the logic that looks
up tu (TopicUser.find_by(...)) and updates tu.notification_level and tu.save!
into the POST/PUT branch, ensure CSRF protection is enabled for the form
submission, and keep perform_show_response only in the mutating handler.
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | ||
|
|
||
| if tu.notification_level > TopicUser.notification_levels[:regular] | ||
| tu.notification_level = TopicUser.notification_levels[:regular] | ||
| else | ||
| tu.notification_level = TopicUser.notification_levels[:muted] | ||
| end | ||
|
|
||
| tu.save! |
There was a problem hiding this comment.
TopicUser.find_by can return nil, causing NoMethodError.
If the user has never interacted with this topic, no TopicUser record exists. find_by returns nil, and line 107 will raise NoMethodError: undefined method 'notification_level' for nil:NilClass.
🐛 Proposed fix: handle missing TopicUser
- tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])
-
- if tu.notification_level > TopicUser.notification_levels[:regular]
- tu.notification_level = TopicUser.notification_levels[:regular]
- else
- tu.notification_level = TopicUser.notification_levels[:muted]
- end
-
- tu.save!
+ tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])
+
+ if tu.nil?
+ TopicUser.change(current_user.id, params[:topic_id],
+ notification_level: TopicUser.notification_levels[:muted])
+ else
+ new_level = if tu.notification_level > TopicUser.notification_levels[:regular]
+ TopicUser.notification_levels[:regular]
+ else
+ TopicUser.notification_levels[:muted]
+ end
+ tu.update!(notification_level: new_level)
+ end🤖 Prompt for AI Agents
In `@app/controllers/topics_controller.rb` around lines 105 - 113,
TopicUser.find_by can return nil, so guard the code that reads/sets
tu.notification_level: after calling TopicUser.find_by(user_id: current_user.id,
topic_id: params[:topic_id]) check whether tu is nil and handle it (either
create a new TopicUser for current_user and params[:topic_id] before toggling
notification_level, or return a suitable response like head :not_found or
redirect), then perform the existing toggle logic using
TopicUser.notification_levels and call tu.save! only when tu is present.
| <% if context_posts.present? %> | ||
| <div class='footer'>%{respond_instructions}</div> | ||
|
|
||
| <hr> | ||
|
|
||
| <% context_posts.each do |p| %> | ||
| <%= render :partial => 'email/post', :locals => {:post => p} %> | ||
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> | ||
|
|
||
| <% context_posts.each do |p| %> | ||
| <%= render partial: 'email/post', locals: { post: p } %> | ||
| <% end %> | ||
| <% end %> | ||
| <% end %> | ||
|
|
||
| <hr> | ||
| <hr> | ||
|
|
||
| <div class='footer'>%{respond_instructions}</div> | ||
| <div class='footer'>%{unsubscribe_link}</div> |
There was a problem hiding this comment.
respond_instructions rendered twice when context_posts is present.
When context_posts.present? is true, %{respond_instructions} appears on both line 6 (inside the conditional) and line 19 (unconditional footer). The email recipient will see the same response instructions duplicated.
Either remove the respond_instructions div on line 6, or wrap the footer instance (line 19) in an unless context_posts.present? block.
Proposed fix: remove the duplicate inside the conditional
<% if context_posts.present? %>
- <div class='footer'>%{respond_instructions}</div>
-
<hr>
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <% if context_posts.present? %> | |
| <div class='footer'>%{respond_instructions}</div> | |
| <hr> | |
| <% context_posts.each do |p| %> | |
| <%= render :partial => 'email/post', :locals => {:post => p} %> | |
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> | |
| <% context_posts.each do |p| %> | |
| <%= render partial: 'email/post', locals: { post: p } %> | |
| <% end %> | |
| <% end %> | |
| <% end %> | |
| <hr> | |
| <hr> | |
| <div class='footer'>%{respond_instructions}</div> | |
| <div class='footer'>%{unsubscribe_link}</div> | |
| <% if context_posts.present? %> | |
| <hr> | |
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> | |
| <% context_posts.each do |p| %> | |
| <%= render partial: 'email/post', locals: { post: p } %> | |
| <% end %> | |
| <% end %> | |
| <hr> | |
| <div class='footer'>%{respond_instructions}</div> | |
| <div class='footer'>%{unsubscribe_link}</div> |
🤖 Prompt for AI Agents
In `@app/views/email/notification.html.erb` around lines 5 - 20, The template
renders %{respond_instructions} twice when context_posts.present? is true; fix
by removing the inner duplicate (the <div
class='footer'>%{respond_instructions}</div> inside the if block) or
alternatively make the bottom footer conditional with unless
context_posts.present? so %{respond_instructions} only appears once; update the
view where context_posts, respond_instructions, and unsubscribe_link are used to
ensure only a single respond_instructions footer is rendered.
|
|
||
| <% context_posts.each do |p| %> | ||
| <%= render :partial => 'email/post', :locals => {:post => p} %> | ||
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> |
There was a problem hiding this comment.
Erroneous dot in HTML class attribute value.
class='.previous-discussion' should be class='previous-discussion'. The leading dot is CSS selector syntax and shouldn't appear in the class attribute itself — any CSS rule targeting .previous-discussion won't match .previous-discussion as a literal class name.
Proposed fix
- <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
+ <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> | |
| <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> |
🤖 Prompt for AI Agents
In `@app/views/email/notification.html.erb` at line 10, In the H4 element in
app/views/email/notification.html.erb (the line rendering t
"user_notifications.previous_discussion"), remove the erroneous leading dot from
the class attribute so the element uses the literal class name
previous-discussion (i.e., change class='.previous-discussion' to
class='previous-discussion') so CSS rules targeting the .previous-discussion
selector will match correctly.
| get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/} | ||
| get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} | ||
| get "t/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} | ||
| get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/} |
There was a problem hiding this comment.
Slugless summary route (line 442) is missing defaults: { summary: true }.
The slug-based summary route on line 439 sets defaults: { summary: true }, but the slugless variant on line 442 does not. This means visiting t/:topic_id/summary will invoke topics#show without params[:summary], so the summary behavior won't activate.
Proposed fix
- get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/}
+ get "t/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/} | |
| get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} | |
| get "t/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} | |
| get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/} | |
| get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/} | |
| get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} | |
| get "t/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} | |
| get "t/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/} |
🤖 Prompt for AI Agents
In `@config/routes.rb` around lines 439 - 442, The slugless summary route "get
\"t/:topic_id/summary\" => \"topics#show\"" is missing defaults so
params[:summary] won't be set; update that route to include defaults: { summary:
true } (preserving the existing constraints: {topic_id: /\d+/}) so topics#show
receives the same summary flag as the slug-based route.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Bug Fixes